Feat: implement ExchangeRateProvider for Czech National Bank#821
Open
deuveme wants to merge 20 commits into
Open
Feat: implement ExchangeRateProvider for Czech National Bank#821deuveme wants to merge 20 commits into
deuveme wants to merge 20 commits into
Conversation
.NET 6 reached EOL in November 2024. Upgrading to .NET 10 LTS which is supported until November 2028. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ZeroTtl guard is unreachable given [Range(1, int.MaxValue)] on CacheTtlSeconds. Currency.Code is already uppercased and trimmed by the constructor, so the ToUpperInvariant()/Trim() calls in NormalizeRequestedCodes were redundant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The CNB txt feed uses comma decimals (cs-CZ) by default, but the English endpoint uses dots. TryParseCnbNumber falls back to InvariantCulture when the token contains a dot but no comma, making the parser endpoint-agnostic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y test ManualResetEventSlim guarantees task1 is holding the refresh semaphore before task2 starts, eliminating the timing dependency on a fixed sleep. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion GetExchangeRates is now a genuine sync method: HttpClient.Send(), SemaphoreSlim.Wait(), and ReadAsStream()+StreamReader instead of their async counterparts. Removes the .GetAwaiter().GetResult() anti-pattern and the using-HttpRequestMessage-before-task-completes bug from SendRequest. IExchangeRateProvider exposes only the sync contract that DotNet.md requires. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- OperationCanceledException instead of TimeoutException in ShouldHandle: HttpClient throws TaskCanceledException on timeout, not TimeoutException - Remove IsNullOrWhiteSpace(currency.Code): Currency constructor already guarantees Code is non-null and non-whitespace - StreamReader with leaveOpen: true so the enclosing using Stream owns disposal and the reader does not double-dispose it Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nforcement - Enable <Nullable>enable</Nullable> and fix all nullable warnings - Fix broken string truncation in CnbDailyTxtParser error messages - Throw ArgumentException on null items in currencies collection (was silently ignored) - Implement IDisposable on ExchangeRateProvider to dispose SemaphoreSlim - Add LogDebug for cache hits and log level config for clean output - Silence HttpClient and Polly info logs, surface Polly warnings Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Implements a concrete IExchangeRateProvider backed by the Czech National Bank (CNB) daily TXT endpoint, restructuring the original skeleton into Domain/Application/Infrastructure layers and adding configuration, resilience, caching, and tests.
Changes:
- Added CNB TXT parsing and domain invariants (
Currency,ExchangeRate,CnbDailyTxtParser). - Implemented an HTTP-based provider with in-memory caching, conditional GET (If-Modified-Since / 304), and retry resilience via
Microsoft.Extensions.Http.Resilience. - Reworked the console entrypoint to use DI + options/configuration, and added a full xUnit test project.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| jobs/Backend/Task/appsettings.json | Adds logging + CNB provider configuration (URL/timeout/retries/cache TTL). |
| jobs/Backend/Task/Program.cs | Switches to Generic Host + DI, wiring options, HttpClient resilience, and CLI currency args. |
| jobs/Backend/Task/Infrastructure/ExchangeRateProvider.cs | Implements CNB-backed provider with caching, 304 handling, concurrency coalescing, and parsing integration. |
| jobs/Backend/Task/Infrastructure/CnbOptionsValidator.cs | Adds URL validation for CNB options. |
| jobs/Backend/Task/Infrastructure/CnbOptions.cs | Defines strongly-typed CNB configuration options with data annotations. |
| jobs/Backend/Task/Infrastructure/CacheSnapshot.cs | Introduces a cache snapshot record for rates + timestamps/Last-Modified. |
| jobs/Backend/Task/ExchangeRateUpdater.sln | Updates solution metadata and includes the new test project. |
| jobs/Backend/Task/ExchangeRateUpdater.csproj | Upgrades target framework and adds required hosting/http/options packages + appsettings copy rules. |
| jobs/Backend/Task/ExchangeRateProvider.cs | Removes old placeholder provider implementation. |
| jobs/Backend/Task/ExchangeRate.cs | Removes old domain model location in favor of layered domain. |
| jobs/Backend/Task/Currency.cs | Removes old currency model location in favor of layered domain. |
| jobs/Backend/Task/Domain/ExchangeRate.cs | Adds validated, immutable exchange rate domain model. |
| jobs/Backend/Task/Domain/Currency.cs | Adds currency normalization and basic invariants. |
| jobs/Backend/Task/Domain/CnbDailyTxtParser.cs | Adds CNB daily.txt parser with decimal normalization and strict validation. |
| jobs/Backend/Task/Application/IExchangeRateProvider.cs | Adds application-layer provider contract. |
| jobs/Backend/Task.Tests/Task.Tests.csproj | Adds xUnit test project referencing the main project. |
| jobs/Backend/Task.Tests/Infrastructure/ExchangeRateProviderTests.cs | Adds extensive tests for caching, 304 behavior, concurrency, and error handling. |
| jobs/Backend/Task.Tests/Infrastructure/CnbOptionsValidatorTests.cs | Adds tests for CNB URL option validation. |
| jobs/Backend/Task.Tests/Domain/DomainInvariantsTests.cs | Adds tests for Currency/ExchangeRate invariants. |
| jobs/Backend/Task.Tests/Domain/CnbDailyTxtParserTests.cs | Adds parser correctness + edge case tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+51
to
+58
| HostApplicationBuilder builder = Host.CreateApplicationBuilder(settings); | ||
|
|
||
| builder.Services | ||
| .AddOptions<CnbOptions>() | ||
| .Bind(builder.Configuration.GetSection(CnbOptions.SectionName)) | ||
| .ValidateDataAnnotations() | ||
| .ValidateOnStart(); | ||
|
|
| [Range(0, int.MaxValue)] | ||
| public int MaxRetries { get; set; } = 3; | ||
|
|
||
| [Range(1, int.MaxValue)] |
| } | ||
|
|
||
| using Stream stream = response.Content.ReadAsStream(); | ||
| using StreamReader reader = new(stream, leaveOpen: true); |
Code reviewFound 2 issues:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Implementation of
ExchangeRateProviderfor the Czech National Bank (CNB) using their public daily exchange rate endpoint.Data source:
https://www.cnb.cz/en/financial_markets/foreign_exchange_market/exchange_rate_fixing/daily.txtDesign decisions
The original skeleton was a single flat file. I reorganised it into three layers to keep concerns separate:
Currency,ExchangeRate,CnbDailyTxtParser. No external dependencies.IExchangeRateProviderinterface. Decouples consumers from the implementation.ExchangeRateProvider, HTTP client, configuration. All I/O lives here.Runtime — Upgraded the project from .NET 6 (original skeleton) to .NET 10. .NET 6 reached end of life in November 2024 and no longer receives security patches. .NET 10 is the current LTS release and also enables modern C# features used throughout the implementation (
<Nullable>enable</Nullable>, collection expressions,ArgumentNullException.ThrowIfNull,TimeProvider).Program — The original skeleton instantiated
ExchangeRateProviderdirectly and printed rates viaConsole.WriteLinewith a hardcoded currency list. Replaced with a proper DI container, structured logging, command-line currency arguments, and exit codes.Implementation highlights
Parser — handles the pipe-delimited format, normalises amounts greater than 1 (e.g. JPY is quoted per 100 units), and supports both comma and dot as decimal separators since CNB uses comma on the Czech endpoint and dot on the English one.
Caching — Results are cached in memory with a configurable TTL (default 1 hour). Concurrent requests are coalesced via
SemaphoreSlimso only one HTTP call is made at a time.If-Modified-Since/ HTTP 304 support avoids re-parsing unchanged data. Cache is preserved on errors so a transient failure does not evict valid data.Resilience — Polly retry pipeline with exponential backoff and jitter on 408, 429, 5xx and network exceptions. All parameters (URL, timeout, retries, TTL) are externalised to
appsettings.jsonand validated at startup.Contract — Respects all three rules from the original skeleton: only rates published by CNB are returned, no inverse rates are calculated, and unknown currencies are silently ignored.
How to run
Tests
Unit tests covering: parser edge cases, TTL and cache behaviour, HTTP 304 handling, concurrency (single HTTP request under concurrent callers), error propagation, snapshot preservation on failure, and domain invariants.
Open questions for the team
Parser strictness — currently a malformed line throws and aborts the entire fetch. An alternative would be to skip invalid lines and log a warning, continuing with the valid ones. Depends on whether partial data is acceptable or not.
Cache TTL — defaulting to 1 hour, but CNB publishes once per day (~14:30 CET). The right value depends on how time-sensitive the rates are for the business.
Stale cache on CNB outage — currently if the CNB is down and the cache has expired, the error propagates to the caller. An alternative would be to serve the last known snapshot regardless of TTL, accepting stale data over no data. Depends on whether partial availability is preferable to a hard failure for the business.
CZK as fixed target currency — all rates are expressed as
X/CZK. If the business needsCZK/X, cross rates between non-CZK currencies, or rates against a different base, the current design would need to change.Single daily fixing — CNB publishes one rate per currency per day. If the business requires bid/ask spreads, intraday rates, or historical lookups, a different data source would be needed.